Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -fno-strict-aliasing to LinuxPPC builds #3087

Merged
merged 1 commit into from
Nov 10, 2018

Conversation

pdbain-ibm
Copy link
Contributor

Applies to LinuxPPC only. This protects against possible functional errors
due to less-than-rigorous casting.
No effect on performance.

Signed-off-by: Peter Bain [email protected]

@pdbain-ibm
Copy link
Contributor Author

@gacholio FYI. @dnakamura would you kindly review?

@pdbain-ibm
Copy link
Contributor Author

pdbain-ibm commented Oct 1, 2018

Hmmm. Found another place (UMA_OPTIMIZATION_CFLAGS) to enable the flag. Pausing this pull request.

@gacholio
Copy link
Contributor

gacholio commented Oct 1, 2018

Maybe we should ignore this until cmake is live. UMA is a "maze of twisty little passages, all alike". This should also be applied to 390 and ARM, if they don't already have it.

@DanHeidinga
Copy link
Member

Maybe we should ignore this until cmake is live.

The compilers are being upgraded (if they haven't been already) and I'm concerned that waiting for cmake may leave us with a tail of odd bugs. My preference is to hit all the points we can, and even if some are missed, we've narrowed the bug window.

@pshipton pshipton changed the title Add -fno-strict-aliasing to LinuxPPC builds WIP Add -fno-strict-aliasing to LinuxPPC builds Oct 1, 2018
@pshipton
Copy link
Member

pshipton commented Oct 1, 2018

@pdbain-ibm renamed the title to include WIP, please remove when you are ready to un-pause.

@@ -128,7 +128,7 @@ ifndef UMA_DO_NOT_OPTIMIZE_CCODE
<#elseif uma.spec.processor.arm>
UMA_OPTIMIZATION_CXXFLAGS += -g -O3 -fno-strict-aliasing $(ARM_ARCH_FLAGS) -Wno-unused-but-set-variable
<#elseif uma.spec.processor.ppc>
UMA_OPTIMIZATION_CXXFLAGS += -O3
UMA_OPTIMIZATION_CXXFLAGS += -O3 -fno-strict-aliasing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to also modify the UMA_OPTIMIZATION_CFLAGS above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, testing that.

@gacholio
Copy link
Contributor

gacholio commented Oct 1, 2018

This appears to be adding the -f to xlC command lines. It should apply only to gcc.

Update both UMA_OPTIMIZATION_CFLAGS and UMA_OPTIMIZATION_CXXFLAGS for GCC
compilation.

Signed-off-by: Peter Bain <[email protected]>
@pdbain-ibm
Copy link
Contributor Author

Added ifdefs to apply the comment only for GCC. Requested a performance run with the option added to the C flags also.

@gacholio
Copy link
Contributor

gacholio commented Oct 2, 2018

It's also worth noting that these changes will have no effect on the JIT or OMR code.

@pdbain-ibm pdbain-ibm changed the title WIP Add -fno-strict-aliasing to LinuxPPC builds Add -fno-strict-aliasing to LinuxPPC builds Oct 12, 2018
@pdbain-ibm
Copy link
Contributor Author

Performance team reports that this has no observable effect on performance.

@gacholio
Copy link
Contributor

What have you done about OMR and JIT?

@pdbain-ibm
Copy link
Contributor Author

I haven't made any changes there.

@gacholio
Copy link
Contributor

I'll leave it up to @DanHeidinga and @pshipton to decide if the half-measure is worth committing.

@DanHeidinga
Copy link
Member

decide if the half-measure is worth committing.

This is a glass half-full, glass half-empty kind of thing. I prefer to see this as a step on the path to get having the option used across the board and as such would be willing to commit it (post review / test of course!)

@pshipton
Copy link
Member

fine with me, although I'd wait until after the 0.11.0 release

@gacholio
Copy link
Contributor

gacholio commented Nov 9, 2018

jenkins test sanity plinux jdk8

@gacholio
Copy link
Contributor

gacholio commented Nov 9, 2018

jenkins compile aix jdk8

@gacholio gacholio merged commit dcad51f into eclipse-openj9:master Nov 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants